-
Notifications
You must be signed in to change notification settings - Fork 80
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Numpy & h5py bug in 1.7.0 #562
Conversation
Signed-off-by: Ian Hoang <[email protected]>
Looks like it was temporarily failing due to latest OpenSearch Docker image. Now it seems to be working |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think rather than bumping to a major version let us pin the numpy to last released 1.x version, which is 1.26.4 and release path for both, pypi and docker.
Couple of questions @rishabh6788:
|
Not in the docker file, but in for 2, mostly related to bumping to major versions as they are more prone to breaking changes and we don't know what will break in future. Pinning it to latest 1.x release will keep the experience same as before. |
Signed-off-by: Ian Hoang <[email protected]>
Signed-off-by: Ian Hoang <[email protected]>
@@ -106,7 +106,7 @@ def str_from_file(name): | |||
"h5py>=3.10.0", | |||
# License: BSD | |||
# Required for knnvector workload | |||
"numpy>=1.24.2", | |||
"numpy>=1.24.2,<=1.26.4", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I beleieve numpy<=1.26.4
would do the job?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previous versions of OSB might have issues with versions earlier than numpy 1.24.2 if I recall correctly. Plus, there are some dependencies above that use this same format as well (such as thespianpy)
@@ -15,7 +15,7 @@ RUN apt-get -y update && \ | |||
RUN groupadd --gid 1000 opensearch-benchmark && \ | |||
useradd -d /opensearch-benchmark -m -k /dev/null -g 1000 -N -u 1000 -l -s /bin/bash benchmark | |||
|
|||
RUN python3 -m pip install h5py==3.10.0; if [ -z "$VERSION" ] ; then python3 -m pip install opensearch-benchmark ; else python3 -m pip install opensearch-benchmark==$VERSION ; fi | |||
RUN if [ -z "$VERSION" ] ; then python3 -m pip install opensearch-benchmark ; else python3 -m pip install opensearch-benchmark==$VERSION ; fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are we removing h5py installation inside dockerfile? Is it because it is already a dependency of OSB installation and will be present as part of installation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, h5py is already included in setup.py and is already a dependency during the OSB installation. If we decide to pin to a version of a dependency, we should do it in setup.py and not the Dockerfile in order to prevent future conflicts.
Signed-off-by: Ian Hoang <[email protected]> Co-authored-by: Ian Hoang <[email protected]>
Description
Version 1.7.0 encountered a bug when we use h5py 3.10.0 and numpy 2.0.0 (which was recently released). Currently, Dockerfile contains a line that forces h5py 3.10.0, which was used to unblock an issue in Jenkins a few months ago. For more, details, see this issue.
h5py 3.10.0 with numpy 2.0.0 results to error
Issues Resolved
#545
Testing
h5py 3.11.0 with numpy 2.0.0
h5py 3.10.0 with numpy 1.26.4
h5py 3.11.0 with numpy 1.26.4
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.